fix(restore): verify backup before extraction and abort on failed restore steps#484
Open
mrrobot47 wants to merge 3 commits into
Open
fix(restore): verify backup before extraction and abort on failed restore steps#484mrrobot47 wants to merge 3 commits into
mrrobot47 wants to merge 3 commits into
Conversation
Harden the restore path against silent data loss when a backup is partial or corrupt and when a destructive step fails: - Verify the downloaded archive before any destructive extraction. The download target was reused if it already existed on disk, so a truncated archive from an interrupted run was silently extracted right after `rm -rf <app>/*`. Compare the local archive size against the remote `rclone size` value (now threaded from pre_restore_check) and run `unzip -t`; re-download once and abort if still invalid. - Check the result of critical restore steps that EE::run_command/EE::exec do not exit on: `wp config set DB_*`, `wp core download`, the `.sql` unzip, and the mysql import. These now run via `ee shell` (whose return code reflects the in-container failure) and abort with EE::error instead of reporting success. The DB import keeps its stderr (2>&1) for a real diagnostic; mysql never echoes the password, so no secret leaks. - Fix the uploads-symlink dance: set $uploads_moved only when the `mv` actually succeeds, and check each subsequent mv/unzip, so a failed move no longer leads to `rm -rf wp-content` destroying the live uploads dir. - Validate meta.json/metadata.json after json_decode (decoded + required keys present/numeric) before use, so a missing/corrupt file no longer silently installs the latest WordPress or bypasses the disk-space and match guards.
Polish the restore-safety hardening based on review: - Drop the dead size comparison in is_backup_archive_valid(): `rclone size` measures the whole remote folder, not the single archive, so it can never detect a truncated archive. `unzip -t` is the sole real integrity guard now; remove the now-unused $remote_backup_size property (the numeric `bytes` validation for the disk-space guard is kept). - Correct the DB-credential comments: escapeshellarg/single-quoting only guards layer-1 word-splitting; the inner `ee shell` `bash -c "$command"` wrapper (a pre-existing limitation) still re-exposes $, backticks and ", so special-char credentials are not fully carried through. Default EE creds are alphanumeric. - Recover the orphaned uploads symlink on the wp-content extraction abort path: recreate wp-content if needed and move the set-aside symlink back before erroring, so a retry can still find it. On the move-back failure path the error message now states where the symlink remains. - Tolerate a DB-less backup in restore_wp(): `unzip` exits 11 when the archive has no sql/ member, which is not an error (integrity already verified); skip the DB import in that case and abort only on other non-zero codes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens ee site restore so it verifies backup integrity before destructive operations and reliably aborts when critical restore steps fail, preventing partial/incorrect restores from being reported as success.
Changes:
- Added archive pre-validation (
unzip -t) with a one-time re-download fallback, aborting on corrupt/truncated backups before any wipe/extract. - Introduced a checked execution helper for in-container commands so failures in
wp/mysqlsteps abort the restore. - Improved safety around wp-content/uploads symlink handling and added metadata validation for
meta.json,metadata.json, andrclone size --json.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // wrapper (a pre-existing limitation) still re-exposes $, backticks and ", so a | ||
| // password containing those is not fully carried through. EE's DB credentials | ||
| // are alphanumeric by default, so this only affects operator-set special chars. | ||
| $restore_command = sprintf( "mysql --skip-ssl -u '%s' -p'%s' -h '%s' '%s' < '%s' 2>&1", $db_user, $db_password, $db_host, $db_name, $sql_path ); |
Member
Author
There was a problem hiding this comment.
Fixed in a2badc6 — restore_db() now uses escapeshellarg() per value, matching get_db_size(). (Layer-1 only; the inner ee shell bash -c "$command" wrapper remains a pre-existing limitation, as noted in the comment.)
Replace restore_db()'s manual single-quoting of the mysql command (`-u '%s' -p'%s' …`) with escapeshellarg() per value, matching get_db_size() and maybe_restore_wp_config(). The manual quoting broke on any credential, host or db-name containing a single quote or whitespace; `-p%s` with escapeshellarg still glues the password to `-p` with no space, as mysql requires. This only fixes layer-1 word-splitting/quote handling; the inner `ee shell` `bash -c "$command"` wrapper (a pre-existing limitation) still cannot carry arbitrary $/backtick/" in a value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes
ee site restorefail safely instead of destroying or half-restoring a live site, and stops it from reporting success when a step actually failed.Fixes
rm -rf <app>/*/ removingwp-content, so a truncated archive from an interrupted run could wipe the site and then fail to restore.ensure_valid_backup_archive()now runsunzip -t(full CRC check) before any destructive step; on failure it re-downloads once, re-tests, and aborts rather than extracting a corrupt archive over the live site. (A size-vs-rclone sizecheck was considered and dropped — that figure covers the whole remote folder, so it can't detect a truncated single archive;unzip -tis the real guard.)EE::run_command()doesn't surface the in-container exit code, sowp config set DB_*,wp core download, and themysqlimport are now run viarun_checked_shell_command()(a childee shellwhose return code reflects the inner failure) and abort on non-zero; the twounzipsteps now check their result too. Previously a failed core download / DB import / extraction still printed "Site restored successfully."$uploads_movedis set only when themvactually succeeds (sorm -rf wp-contentcan't delete the live uploads), each subsequent step is checked, and on the abort pathrestore_moved_uploads()puts the set-aside symlink back so a failed restore doesn't strand the site's uploads reference.meta.json(WP version),metadata.json(site-type/public-dir match), andrclone size --json(bytes) are validated for decode + required keys before use — closing the silent paths where an empty version installed the latest WordPress andnull['bytes']made the disk-space guard a no-op.A DB-less backup is handled gracefully (the SQL
unzipexit 11 = "no member" is treated as "no DB", matchingrestore_site), and the DB-cred comments are accurate about the pre-existingbash -c "…"shell layer (escapeshellarg guards layer-1 word-splitting only; EE creds are alphanumeric by default).Verification
php -lpasses. Reviewed by two independent reviewers (correctness + design); both empirically confirmed (in containers) thatunzip -trejects truncation at every offset, thatee shellpropagates the inner exit code, and that no path destroys/overwrites live files before verification. No merge-blocker; trial-merges cleanly with the backup-integrity PR (#483).